Take the vertical coordinates from the data and the grid; drop layer/interface - #7
Merged
Merged
Conversation
`convergent_transport` validated its `layer`/`interface` arguments with
`layer.replace("l", "i") != interface`, which rewrites every "l" in the
name. That rejected any consistent pair whose stem contains an "l"
("lam_l"/"lam_i", "level_l"/"level_i") and accepted unrelated pairs whose
substitution happened to coincide ("ml_l"/"mi_i").
Ask the grid instead: accept the pair if any of its axes registers `layer`
at "center" and `interface` at an interface position (outer/inner/left/
right). Grids handed to sectionate usually declare only their horizontal
axes (as `examples/load_example_model_grid.py` does), so a pair the grid
says nothing about falls back to the `<stem>l`/`<stem>i` convention --
anchored at the end of the name rather than substituted throughout. The
error now names both arguments and the axes the grid does offer.
Fixes MOM6-community#51
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 2, 2026
`convergent_transport` made the caller name the vertical coordinates even when the grid already knew them. `xwmb` shows what that costs: it builds the arguments as `grid.axes["Z"].coords['center']` / `['outer']` and hands them back as strings, which sectionate then re-derives the relationship of. Read them off the axis directly instead, and let the caller stay silent. An explicit name that contradicts the grid's Z axis now raises rather than overriding it: a caller who disagrees with the grid about which vertical coordinate its transports live on is confused, and quietly preferring either one would label the output with a coordinate that need not describe the data. Names that agree stay valid, which is how such callers invoke it. The defaults change from "z_l"/"z_i" to None. That default was unusable anyway -- it looks up `grid._ds["z_l"]` and so raised KeyError on any grid without that variable, which is why all 32 call sites in this tree pass `layer=` explicitly and none relies on it. Grids that declare no vertical axis and pass no names now get output with no vertical coordinate instead of that KeyError. Fixes MOM6-community#51 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All five run cleanly against this branch's code. Results are unchanged: comparing every textual output against the previous ones, the only differences are the two artifacts below. Notebook 5's overturning streamfunction reproduces bit-for-bit (psi range (Sv): -58.09992975038242 to 53.35870875408872). Two things in these outputs are artifacts of how they were produced, not of the change under review: 1. The version banner reads "Sectionate version: 0.4.0rc2.dev1" rather than a release number. hatch-vcs derives the version from the git tag, and this branch is untagged and was installed editable, so it resolves to a .devN string off the last tag. It will read a real version again once the notebooks are refreshed from a tagged release. 2. Warning messages cite paths under a build worktree (/Users/hfdrake/code/wt-sectionate-layer-iface/...) instead of a normal checkout, and their line numbers reflect this branch's transports.py. Both would be resolved by a refresh on a tagged release in a normal checkout; neither reflects anything about the code being reviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Both branches committed re-executed example notebooks, so all five conflict. Neither branch changes any notebook result -- #8's notebook refresh is a no-op by construction, and notebook 5's streamfunction is bit-identical on both -- so the conflict is entirely re-executed output. Take this branch's copies wholesale (`--ours`) rather than hand-merging notebook JSON; they are re-executed on the merge result in a later commit, which is what actually reconciles them. Everything else merged cleanly: #8 touches gridutils.py and two test files that this branch does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`convergent_transport` asked the caller to name the vertical layer and interface
coordinates, then checked the two names against each other by string substitution
(`layer.replace("l", "i") != interface`, MOM6-community#51). Both the check and the arguments
turn out to be unnecessary.
The layer coordinate is a dimension of `utr`/`vtr` themselves, so it already rides
along into the section: on the CM4p25 example file `umo` has dims
`(time, sigma2_l, yh, xq)` and carries `sigma2_l`, and `dsout[layer] =
grid._ds[layer]` was re-attaching a coordinate that was on the output either way.
The interface coordinate genuinely cannot ride along -- it is one point longer than
the layer coordinate, so it is a dimension of nothing on the section -- but the grid
knows it, and knows it without being told which coordinate the transports are
resolved over: whichever axis registers their layer dimension at its "center"
position names the interface at "outer"/"inner"/"left"/"right". `_interface_coords`
reads it off from there. Keying on the data's own dimensions rather than on an axis
named "Z" means a vertical axis under any name is found, and grids that declare only
their horizontal axes (most of them, since sections are traced horizontally) simply
contribute nothing.
So both parameters are removed rather than re-validated, and the `<stem>l`/`<stem>i`
convention leaves the source entirely. Names now play no part at all: the pairing is
the grid's, whatever it spells.
Verified on the real CM4p25 OSNAP-west section: with the vertical axis declared on
the example grid, `convergent_transport(grid, i_c, j_c)` returns output that is
`xr.testing.assert_identical` to the previous call passing
`layer="sigma2_l", interface="sigma2_i"`.
All 32 in-tree call sites passed `layer=` explicitly and are updated. The 19 tests
covering the old validation are replaced by 11 covering the new behavior: the layer
coordinate arriving from the transports, the interface coordinate from the grid,
names following no convention, a vertical axis not named "Z", an interface position
naming a bare dimension with no values, transports with no vertical dimension at
all, and `TypeError` for both removed keywords.
Downstream callers that read the two names off `grid.axes["Z"].coords` only to hand
them back (xwmb does exactly this) can drop them; that needs its own PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Notebooks 2, 3 and 5 drop the `layer=`/`interface=` arguments, and the CM4p25 example grid now declares its vertical axis, so notebooks 2 and 3 still get `sigma2_i` on their output -- from the grid rather than from a string. All five were re-executed together in one env (xgcm 0.10.1, this branch installed editable), which is also what finally reconciles the merge: the committed outputs are now the ones this branch's code actually produces, rather than either side's copy taken wholesale. No numerical output changed. Comparing every textual output against the previously committed ones, the only differences are the version banner (`0.4.0rc2.dev1` -> `0.4.0rc2.dev3`, a hatch-vcs dev string from an editable install of an untagged branch) and warning messages citing this checkout's paths. Notebook 5's overturning streamfunction reproduces bit-for-bit: `psi range (Sv): -58.09992975038242 to 53.35870875408872`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hdrake
added a commit
to hdrake/xwmb
that referenced
this pull request
Aug 4, 2026
Sectionate is removing both arguments (hdrake/sectionate#7): the layer coordinate is a dimension of the transports it is handed, so it reaches the output on its own, and the matching interface coordinate is read off whichever grid axis registers that dimension at its "center" position -- which is where `_convergence_along_section` read the two names from before handing them straight back. No behavioral change. The layer coordinate arrives either way, whatever grid sectionate is handed -- and this call hands it `hgrid`, which has no vertical axis at all, so the `interface=` name was the only thing supplying one. It never survived the call site regardless: the interface coordinate sits on its own dimension, so extracting `[...]["conv_mass_transport"]` drops it. Nothing downstream reads it; `transform_to_lambda` takes its target and target_data from `grid._ds`. Verified rather than argued: with sectionate's branch installed, the suite is 57 passed / 0 skipped / 0 failed, the three real-CM4p25-data tests included. Reverting just this hunk against the same sectionate fails 4 tests with `TypeError: ... unexpected keyword argument 'layer'`, so the along-section path these tests cover is genuinely this call. Must not merge before hdrake/sectionate#7 reaches sectionate `master`: until then `convergent_transport` still defaults to `layer="z_l", interface="z_i"`, which would mislabel a non-depth grid's output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes MOM6-community#51.
convergent_transportasked the caller to name the vertical layer and interface coordinates, then validated the pair by string substitution:str.replacerewrites every"l", so the check only worked for stems containing none:lam_l/lam_iandlevel_l/level_iwere rejected outright, while unrelated pairs whose substitution happened to coincide (ml_l/mi_i) were silently accepted.This PR started as a better check. It ends by deleting the question: both parameters are gone.
Why the arguments were never needed
utr/vtrthemselves. On the CM4p25 example fileumohas dims(time, sigma2_l, yh, xq)and carriessigma2_lwith its values, so it is on the output whether or not anyone names it;dsout[layer] = grid._ds[layer]was re-attaching a coordinate that was already there."center"position names the interface at"outer"/"inner"/"left"/"right".So the names were never carrying information the call did not already have.
_interface_coordsreads the interface off the grid, keyed on the data's own dimensions rather than on an axis called"Z"— so a vertical axis under any name is found, and a grid declaring only its horizontal axes (most of them, since sections are only traced horizontally) simply contributes nothing.The
<stem>l/<stem>iconvention leaves the source entirely. Names now play no part: the pairing is the grid's, whatever it spells.API change
layer=andinterface=are removed, not deprecated — pre-1.0, and the oldlayer="z_l"default was already a landmine (it didgrid._ds["z_l"], so it raisedKeyErroron any grid without that variable). All 32 in-tree call sites passedlayer=explicitly; every one is updated.Downstream callers that read the two names off
grid.axes["Z"].coordsonly to hand them straight back —xwmb/budget.pydoes exactly this — can now delete those two dict entries. That is hdrake/xwmb#44, open as a draft: the two repos are in lockstep, sincexwmbdepends onsectionate@master, so it merges immediately after this reachesmasterand not before.Verification
convergent_transport(grid, i_c, j_c)on this head isxr.testing.assert_identicalto the previous call passinglayer="sigma2_l", interface="sigma2_i"— same transports, samesigma2_l, samesigma2_i.MyCenters/MyEdges); a vertical axis not named"Z"; an interface position naming a bare dimension with no coordinate values; transports with no vertical dimension; andTypeErrorfor each removed keyword.0.4.0rc2.dev1→0.4.0rc2.dev3, a hatch-vcs dev string from an editable install of an untagged branch) and warning messages citing this checkout's paths. Notebook 5's overturning streamfunction reproduces bit-for-bit:psi range (Sv): -58.09992975038242 to 53.35870875408872.Examples
examples/load_example_model_grid.pynow declares the CM4p25 grid's vertical axis ('Z': {'center': 'sigma2_l', 'outer': 'sigma2_i'}), so notebooks 2 and 3 still getsigma2_ion their output — from the grid rather than from a string. Declaring it is only possible because of #8: before that fixbuild_neighbor_mapspadded over every axis the grid carried, so a grid with a vertical axis could not be traced at all.Merge conflict with #8, resolved
#8 is merged, and both branches had committed re-executed notebooks, so all five conflicted. Resolved by taking this branch's copies wholesale (
--ours) rather than hand-merging notebook JSON, then re-executing all five on the merge result — so the committed outputs are the ones this branch's code actually produces, not either side's stale copy. Everything else merged cleanly; #8 touchesgridutils.pyand two test files this branch does not.Drafted with AI assistance (Claude Code). I have read the diff and run the tests, the notebooks, and the no-op check myself.